feat(lyrics): major lyrics improvement#3630
Open
ibratabian17 wants to merge 15 commits into
Open
Conversation
* add support for TTML Parser and LyricsPlus
* use apple music android-like positioning, but adapt it to metrolist positioning-like system where it's not fixed by agent id, but it's dynamic based of agent change
* The current implementation splits the text into individual UTF-16 code units (characters), which breaks grapheme clusters in Indic scripts like Malayalam (where base characters and combining marks must stay together).
* This improving UX
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional per-line endTime and mutable linePosition to LyricsEntry; threads agent metadata through TTML parsing and LRC generation; removes agent header lines during parsing; prefers endTime for active-line/end computations; applies agent-based positioning; refactors rendering to Unicode-cluster drawing and visibility-aware composition/scrolling. Changes
Sequence Diagram(s)sequenceDiagram
participant Source as Lyrics Source\n(TTML / JSON)
participant Provider as LyricsPlusProvider
participant Parser as TTMLParser
participant Utils as LyricsUtils
participant VM as LyricsViewModel
participant UI as ExperimentalLyrics / LyricsLine
Source->>Provider: supply raw payload
Provider->>Parser: parse TTML (collect agentsMap)
Parser-->>Provider: parsed lines + agentsMap (+endTime)
Provider->>Utils: pass parsed entries + agentsMap
Utils-->>Utils: remove agent headers, set linePosition, extract endTime
Utils-->>VM: processed entries stream
VM-->>VM: compute currentEnd (prefers endTime)
VM-->>UI: emit entries with endTime/linePosition
UI->>UI: compute visibility window, compose visible items
UI->>LyricsLine: render entry using clusters & agent alignment
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
betterlyrics/src/main/kotlin/com/metrolist/music/betterlyrics/TTMLParser.kt (1)
321-348:⚠️ Potential issue | 🟡 MinorPreserve agent id casing between headers and line tags.
Headers are emitted with the original
agentskeys, but line tags useline.agent?.lowercase().LyricsUtilsstores metadata by header id and later looks upentry.agent, so[agent:Lead:group:...]plus{agent:lead}loses the metadata and can mis-position group/other agents.Proposed fix
- lines.forEach { line -> - line.agent?.lowercase()?.let { raw -> + lines.forEach { line -> + line.agent?.takeIf { it.isNotBlank() }?.let { raw -> if (!agentMap.containsKey(raw)) { agentMap[raw] = raw } } } @@ - val agentId = agentMap[line.agent?.lowercase()] + val agentId = agentMap[line.agent]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@betterlyrics/src/main/kotlin/com/metrolist/music/betterlyrics/TTMLParser.kt` around lines 321 - 348, The agent headers use the original keys from the agents map but line tags are built from line.agent?.lowercase(), causing mismatched IDs; change how agentMap is populated in toLRC: when iterating lines, compute a lowercase key (e.g., val key = line.agent?.lowercase()) but store the original agent id string as the value (e.g., agentMap[key] = line.agent) so that later tag generation uses the original-cased id (agentMap[line.agent?.lowercase()]) and headers remain unchanged; ensure null-safety (only insert when line.agent is not null/blank) so lookups produce the correctly-cased id for {agent:...} tags.app/src/main/kotlin/com/metrolist/music/ui/component/LyricsLine.kt (1)
496-506:⚠️ Potential issue | 🟠 MajorUse union bounds for grapheme clusters.
The cluster renderer still relies on per-code-unit geometry: RTL highlighting checks only
range.first, while LTR width sums every code unit’s bounding box. For combined characters/surrogate pairs, this can under-highlight or over-shift the exact cases this PR is trying to fix.Proposed direction
- clusters.forEach { (range, _) -> - val i = range.first - if (wordIdxMap[i] == wIdx) { - val bounds = layoutResult.getBoundingBox(i) - left = minOf(left, bounds.left) - right = maxOf(right, bounds.right) - top = minOf(top, bounds.top) - bottom = maxOf(bottom, bounds.bottom) - found = true - } - } + clusters.forEach { (range, _) -> + val i = range.first + if (wordIdxMap[i] == wIdx) { + for (idx in range) { + val bounds = layoutResult.getBoundingBox(idx) + left = minOf(left, bounds.left) + right = maxOf(right, bounds.right) + top = minOf(top, bounds.top) + bottom = maxOf(bottom, bounds.bottom) + } + found = true + } + } @@ - var clusterWidth = 0f + var clusterLeft = Float.MAX_VALUE + var clusterRight = Float.MIN_VALUE for (idx in range) { - clusterWidth += layoutResult.getBoundingBox(idx).width + val bounds = layoutResult.getBoundingBox(idx) + clusterLeft = minOf(clusterLeft, bounds.left) + clusterRight = maxOf(clusterRight, bounds.right) } + val clusterWidth = clusterRight - clusterLeftAlso applies to: 599-620
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/component/LyricsLine.kt` around lines 496 - 506, The code currently only uses range.first when checking/highlighting clusters, causing per-code-unit geometry errors for grapheme clusters; update both places (the block using clusters.forEach { (range, _) -> ... } and the similar block later) to compute union bounds by iterating all code unit indices in the cluster range (e.g., for i in range.first..range.last) and call layoutResult.getBoundingBox(i) for each index, then min/max left/right/top/bottom across those boxes to produce a single unioned bounding rect before marking found/highlighting/shift logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/lyrics/LyricsUtils.kt`:
- Around line 452-460: The filter over rawLines currently uses
AGENT_HEADER_REGEX.find(trimmed) which matches any line containing an agent
header anywhere and incorrectly drops lyric lines that include “[agent:...]”
inline; change the condition in the rawLines.filter within the code that builds
lines/agents (the block that references AGENT_HEADER_REGEX, agents,
LyricsAgentMetadata and alias/type/name) to only treat the line as metadata when
the entire trimmed line exactly matches the agent header (e.g., use a full-match
check rather than find), so only whole-agent-header lines are removed and inline
occurrences remain in the returned lines.
In `@app/src/main/kotlin/com/metrolist/music/ui/component/ExperimentalLyrics.kt`:
- Around line 767-773: The visibility state uses remember { derivedStateOf { ...
} } and captures the initial maxHeightPx so it doesn't update on
rotation/resizing; change the remember call to include maxHeightPx as a key
(remember(maxHeightPx) { derivedStateOf { val top = animatedOffset +
userManualOffset; top > -2500f && top < maxHeightPx + 2500f } }) so the derived
state (isVisible) recomputes when maxHeightPx changes; update the block
referencing isVisible, derivedStateOf, remember, maxHeightPx, animatedOffset and
userManualOffset accordingly.
In `@betterlyrics/src/main/kotlin/com/metrolist/music/betterlyrics/TTMLParser.kt`:
- Around line 180-181: The code currently sets endTime to
parseTime(timingAttr(p, "end")) + offset which treats a missing end attribute
(timingAttr returning "") as 0.0; change this by first reading val end =
timingAttr(p, "end") and then if end.isBlank() set endTime to null (use a
nullable Double) otherwise set endTime = parseTime(end) + offset so TTML lines
without an explicit <p end> keep a null endTime; update the variable
declaration/type where endTime is defined and adjust any downstream usage to
handle the nullable endTime.
---
Outside diff comments:
In `@app/src/main/kotlin/com/metrolist/music/ui/component/LyricsLine.kt`:
- Around line 496-506: The code currently only uses range.first when
checking/highlighting clusters, causing per-code-unit geometry errors for
grapheme clusters; update both places (the block using clusters.forEach {
(range, _) -> ... } and the similar block later) to compute union bounds by
iterating all code unit indices in the cluster range (e.g., for i in
range.first..range.last) and call layoutResult.getBoundingBox(i) for each index,
then min/max left/right/top/bottom across those boxes to produce a single
unioned bounding rect before marking found/highlighting/shift logic.
In `@betterlyrics/src/main/kotlin/com/metrolist/music/betterlyrics/TTMLParser.kt`:
- Around line 321-348: The agent headers use the original keys from the agents
map but line tags are built from line.agent?.lowercase(), causing mismatched
IDs; change how agentMap is populated in toLRC: when iterating lines, compute a
lowercase key (e.g., val key = line.agent?.lowercase()) but store the original
agent id string as the value (e.g., agentMap[key] = line.agent) so that later
tag generation uses the original-cased id (agentMap[line.agent?.lowercase()])
and headers remain unchanged; ensure null-safety (only insert when line.agent is
not null/blank) so lookups produce the correctly-cased id for {agent:...} tags.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ab53d12-8d58-4ae3-81b1-a8cdc1e931e2
📒 Files selected for processing (8)
app/src/main/kotlin/com/metrolist/music/lyrics/LyricsEntry.ktapp/src/main/kotlin/com/metrolist/music/lyrics/LyricsPlusProvider.ktapp/src/main/kotlin/com/metrolist/music/lyrics/LyricsUtils.ktapp/src/main/kotlin/com/metrolist/music/ui/component/ExperimentalLyrics.ktapp/src/main/kotlin/com/metrolist/music/ui/component/LyricsLine.ktapp/src/main/kotlin/com/metrolist/music/viewmodels/LyricsViewModel.ktbetterlyrics/src/main/kotlin/com/metrolist/music/betterlyrics/TTMLParser.ktpaxsenix/src/main/kotlin/com/metrolist/paxsenix/Paxsenix.kt
Comment on lines
+452
to
+460
| val lines = rawLines.filter { line -> | ||
| val trimmed = line.trim() | ||
| val agentMatch = AGENT_HEADER_REGEX.find(trimmed) | ||
| if (agentMatch != null) { | ||
| val alias = agentMatch.groupValues[1] | ||
| val type = agentMatch.groupValues[2].ifEmpty { "person" } | ||
| val name = agentMatch.groupValues[3] | ||
| agents[alias] = LyricsAgentMetadata(alias, type, name) | ||
| false |
Contributor
There was a problem hiding this comment.
Only strip full agent-header lines.
Using find() drops any lyric line that merely contains [agent:...] in its text. Treat it as metadata only when the trimmed line is exactly the header.
Proposed fix
- val agentMatch = AGENT_HEADER_REGEX.find(trimmed)
+ val agentMatch = AGENT_HEADER_REGEX.matchEntire(trimmed)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/lyrics/LyricsUtils.kt` around lines
452 - 460, The filter over rawLines currently uses
AGENT_HEADER_REGEX.find(trimmed) which matches any line containing an agent
header anywhere and incorrectly drops lyric lines that include “[agent:...]”
inline; change the condition in the rawLines.filter within the code that builds
lines/agents (the block that references AGENT_HEADER_REGEX, agents,
LyricsAgentMetadata and alias/type/name) to only treat the line as metadata when
the entire trimmed line exactly matches the agent header (e.g., use a full-match
check rather than find), so only whole-agent-header lines are removed and inline
occurrences remain in the returned lines.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/kotlin/com/metrolist/music/lyrics/LyricsPlusProvider.kt (1)
353-362:⚠️ Potential issue | 🟡 MinorBackground line end-time is derived from the main line's duration.
At line 358 the background line is emitted with
bgTime(the earliest bg word time) as its start but withline.duration(the main line's duration) as its length, so the trailing timestamp becomesbgTime + line.duration. SincebgTimetypically starts later thanline.time, this overshoots the actual background range (and may extend past the main line's end). If the UI uses this end-time to determine when the bg line is "active", it will be marked active for too long.Prefer computing the bg range from the bg words themselves, e.g.
bgWords.maxOf { it.time + it.duration } - bgTime, and fall back to0soappendLrcLinewon't emit a trailing timestamp when it's unknown.♻️ Suggested fix
- val bgTime = bgToEmit.minOf { it.time } - val bgTag = if (lastWasBg) "" else "{bg}" - sb.appendLrcLine(bgTime, line.duration, bgTag, bgText) + val bgTime = bgToEmit.minOf { it.time } + val bgEnd = bgToEmit.maxOf { it.time + it.duration } + val bgDuration = (bgEnd - bgTime).coerceAtLeast(0) + val bgTag = if (lastWasBg) "" else "{bg}" + sb.appendLrcLine(bgTime, bgDuration, bgTag, bgText)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/lyrics/LyricsPlusProvider.kt` around lines 353 - 362, The background line is using the main line's duration (line.duration) which makes the emitted end timestamp incorrect; change the duration passed to appendLrcLine so it is calculated from the bg words themselves: compute bgTime = bgToEmit.minOf { it.time } and bgEnd = bgToEmit.maxOf { it.time + it.duration }, then use bgDuration = max(0, bgEnd - bgTime) (fall back to 0 to avoid emitting a trailing timestamp) when calling appendLrcLine instead of line.duration; keep the existing isWordSync/appendWordBlock and lastWasBg logic intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/lyrics/LyricsPlusProvider.kt`:
- Around line 318-322: The agent metadata headers in LyricsPlusProvider
currently use the map key from response.metadata?.agents (alias variable) which
can differ from AgentInfo.alias and break inline agent tag resolution; update
the foreach over response.metadata?.agents to use info.alias (falling back or
skipping when blank) as the header identifier and keep the existing fields
(info.type / info.name) when appending to sb so headers match the inline tags
produced later (e.g., by line.element.singer).
- Around line 372-381: The appendLrcLine function currently always appends a
trailing timestamp when durationMs > 0; update it to accept an additional
Boolean parameter (e.g., isWordSync) and skip appending the end timestamp
(formatLrcTime(timeMs + durationMs)) when isWordSync is true to match
TTMLParser.toLRC behavior and avoid redundant timestamps that
LyricsUtils.parseLine already tolerates; also update the KDoc to describe that
the trailing timestamp is conditional based on isWordSync. Ensure callers
(notably TTMLParser.toLRC) are updated to pass the correct isWordSync flag when
invoking appendLrcLine.
---
Outside diff comments:
In `@app/src/main/kotlin/com/metrolist/music/lyrics/LyricsPlusProvider.kt`:
- Around line 353-362: The background line is using the main line's duration
(line.duration) which makes the emitted end timestamp incorrect; change the
duration passed to appendLrcLine so it is calculated from the bg words
themselves: compute bgTime = bgToEmit.minOf { it.time } and bgEnd =
bgToEmit.maxOf { it.time + it.duration }, then use bgDuration = max(0, bgEnd -
bgTime) (fall back to 0 to avoid emitting a trailing timestamp) when calling
appendLrcLine instead of line.duration; keep the existing
isWordSync/appendWordBlock and lastWasBg logic intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 72078a42-850f-40f2-a225-d378038a1e76
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/lyrics/LyricsPlusProvider.kt
Comment on lines
372
to
381
| /** Appends `[mm:ss.cc]<tag>text\n` */ | ||
| private fun StringBuilder.appendLrcLine(timeMs: Long, tag: String, text: String) { | ||
| private fun StringBuilder.appendLrcLine(timeMs: Long, durationMs: Long, tag: String, text: String) { | ||
| append(formatLrcTime(timeMs)) | ||
| append(tag) | ||
| append(text) | ||
| if (durationMs > 0) { | ||
| append(formatLrcTime(timeMs + durationMs)) | ||
| } | ||
| append('\n') | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the LyricsPlusProvider.kt file
find . -name "LyricsPlusProvider.kt" -type fRepository: MetrolistGroup/Metrolist
Length of output: 138
🏁 Script executed:
# Once located, read the file around lines 372-381 and context
# Get a sense of the file size first
find . -name "LyricsPlusProvider.kt" | xargs wc -lRepository: MetrolistGroup/Metrolist
Length of output: 142
🏁 Script executed:
# Read the appendLrcLine function and surrounding context
sed -n '350,390p' ./app/src/main/kotlin/com/metrolist/music/lyrics/LyricsPlusProvider.ktRepository: MetrolistGroup/Metrolist
Length of output: 1682
🏁 Script executed:
# Also check lines 340-365 to see the word-sync usage context
sed -n '340,365p' ./app/src/main/kotlin/com/metrolist/music/lyrics/LyricsPlusProvider.ktRepository: MetrolistGroup/Metrolist
Length of output: 1238
🏁 Script executed:
# Check the KDoc on line 372
sed -n '372,375p' ./app/src/main/kotlin/com/metrolist/music/lyrics/LyricsPlusProvider.ktRepository: MetrolistGroup/Metrolist
Length of output: 273
🏁 Script executed:
# Find TTMLParser.kt to verify the claim about omitting trailing time
find . -name "TTMLParser.kt" -type fRepository: MetrolistGroup/Metrolist
Length of output: 145
🏁 Script executed:
# Search for the toLRC method in TTMLParser
grep -n "fun toLRC" ./betterlyrics/src/main/kotlin/com/metrolist/music/betterlyrics/TTMLParser.ktRepository: MetrolistGroup/Metrolist
Length of output: 165
🏁 Script executed:
# Check the specific code in TTMLParser.toLRC regarding word-sync and trailing timestamps
sed -n '420,480p' ./betterlyrics/src/main/kotlin/com/metrolist/music/betterlyrics/TTMLParser.ktRepository: MetrolistGroup/Metrolist
Length of output: 50
🏁 Script executed:
# Read the toLRC method implementation
sed -n '321,380p' ./betterlyrics/src/main/kotlin/com/metrolist/music/betterlyrics/TTMLParser.ktRepository: MetrolistGroup/Metrolist
Length of output: 2427
🏁 Script executed:
# Get more context - check file size
wc -l ./betterlyrics/src/main/kotlin/com/metrolist/music/betterlyrics/TTMLParser.ktRepository: MetrolistGroup/Metrolist
Length of output: 149
🏁 Script executed:
# Search for LyricsEntry and LyricsViewModel to find the extended-LRC parser
find . -name "LyricsEntry.kt" -o -name "LyricsViewModel.kt" | head -5Repository: MetrolistGroup/Metrolist
Length of output: 203
🏁 Script executed:
# Search for extended LRC parsing or word block parsing
rg -l "word.*sync|<word:|extended.*lrc" --type kt | head -10Repository: MetrolistGroup/Metrolist
Length of output: 94
🏁 Script executed:
# Search for LRC parsing logic in LyricsEntry and LyricsViewModel
rg "word\:|<word|parseL|lrc.*pars" ./app/src/main/kotlin/com/metrolist/music/lyrics/LyricsEntry.kt ./app/src/main/kotlin/com/metrolist/music/viewmodels/LyricsViewModel.kt -A 3Repository: MetrolistGroup/Metrolist
Length of output: 167
🏁 Script executed:
# Get file sizes to understand scope
wc -l ./app/src/main/kotlin/com/metrolist/music/lyrics/LyricsEntry.kt ./app/src/main/kotlin/com/metrolist/music/viewmodels/LyricsViewModel.ktRepository: MetrolistGroup/Metrolist
Length of output: 224
🏁 Script executed:
# Search for word-sync or extended-LRC parsing with simpler patterns
rg "word|<.*>|parse" ./app/src/main/kotlin/com/metrolist/music/lyrics/LyricsEntry.kt -A 2 -B 2Repository: MetrolistGroup/Metrolist
Length of output: 571
🏁 Script executed:
# Also check LyricsViewModel
rg "word|<.*>|parse" ./app/src/main/kotlin/com/metrolist/music/viewmodels/LyricsViewModel.kt -A 2 -B 2Repository: MetrolistGroup/Metrolist
Length of output: 1669
🏁 Script executed:
# Find LyricsUtils class
find . -name "LyricsUtils.kt" -o -name "LyricsUtils.java"Repository: MetrolistGroup/Metrolist
Length of output: 131
🏁 Script executed:
# Search for parseLyrics function
rg "fun parseLyrics" --type-listRepository: MetrolistGroup/Metrolist
Length of output: 457
🏁 Script executed:
# Read LyricsUtils.kt to find parseLyrics
wc -l ./app/src/main/kotlin/com/metrolist/music/lyrics/LyricsUtils.ktRepository: MetrolistGroup/Metrolist
Length of output: 136
🏁 Script executed:
# Search for parseLyrics function
rg "fun parseLyrics" ./app/src/main/kotlin/com/metrolist/music/lyrics/LyricsUtils.kt -A 50Repository: MetrolistGroup/Metrolist
Length of output: 2108
🏁 Script executed:
# Continue reading parseLyrics to see word block parsing
rg "fun parseLyrics" ./app/src/main/kotlin/com/metrolist/music/lyrics/LyricsUtils.kt -A 100 | head -120Repository: MetrolistGroup/Metrolist
Length of output: 3838
🏁 Script executed:
# Search for word block parsing logic (looking for <word patterns)
rg "RICH_SYNC|\\<word|WordTimestamp" ./app/src/main/kotlin/com/metrolist/music/lyrics/LyricsUtils.kt -B 2 -A 2Repository: MetrolistGroup/Metrolist
Length of output: 174
🏁 Script executed:
# Search for word block parsing logic - fix regex escaping
rg "RICH_SYNC|word|WordTimestamp" ./app/src/main/kotlin/com/metrolist/music/lyrics/LyricsUtils.kt -B 2 -A 2 | head -80Repository: MetrolistGroup/Metrolist
Length of output: 3853
🏁 Script executed:
# Search for parseRichSyncLyrics to see how word blocks are handled
rg "fun parseRichSyncLyrics" ./app/src/main/kotlin/com/metrolist/music/lyrics/LyricsUtils.kt -A 80Repository: MetrolistGroup/Metrolist
Length of output: 4712
🏁 Script executed:
# Search for parseStandardLyrics to see how standard LRC format is parsed
rg "fun parseStandardLyrics" ./app/src/main/kotlin/com/metrolist/music/lyrics/LyricsUtils.kt -A 60Repository: MetrolistGroup/Metrolist
Length of output: 2624
🏁 Script executed:
# Also search for TRAILING_TIME_REGEX definition
rg "TRAILING_TIME_REGEX" ./app/src/main/kotlin/com/metrolist/music/lyrics/LyricsUtils.ktRepository: MetrolistGroup/Metrolist
Length of output: 856
Update appendLrcLine to omit trailing timestamp for word-sync lines and fix stale KDoc.
appendLrcLine unconditionally appends [end] whenever durationMs > 0, but TTMLParser.toLRC deliberately omits the trailing timestamp when word-sync data is present. While LyricsUtils.parseLine tolerates and strips trailing timestamps with word blocks, this creates an inconsistency and unnecessary output. For consistency, skip the trailing timestamp when isWordSync is true. The KDoc on line 372 is also stale and should be updated to reflect the conditional trailing-timestamp behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/lyrics/LyricsPlusProvider.kt` around
lines 372 - 381, The appendLrcLine function currently always appends a trailing
timestamp when durationMs > 0; update it to accept an additional Boolean
parameter (e.g., isWordSync) and skip appending the end timestamp
(formatLrcTime(timeMs + durationMs)) when isWordSync is true to match
TTMLParser.toLRC behavior and avoid redundant timestamps that
LyricsUtils.parseLine already tolerates; also update the KDoc to describe that
the trailing timestamp is conditional based on isWordSync. Ensure callers
(notably TTMLParser.toLRC) are updated to pass the correct isWordSync flag when
invoking appendLrcLine.
Use union bounds for grapheme clusters. The cluster renderer still relies on per-code-unit geometry: RTL highlighting checks only range.first, while LTR width sums every code unit’s bounding box. For combined characters/surrogate pairs, this can under-highlight or over-shift the exact cases this PR is trying to fix.
…headers to match inline agent tags. The inline agent tags at lines 343–344 use identifiers from line.element.singer (e.g., "v1"), but the metadata headers at lines 320–322 use the metadata.agents map key directly. If the API response keys that map by singer ID or other identifiers rather than by alias, the header identifiers won't correlate with the inline tags, breaking downstream agent resolution. * lyricsplus normalize voice1 to v1 on alias
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/src/main/kotlin/com/metrolist/music/lyrics/LyricsPlusProvider.kt (1)
373-380:⚠️ Potential issue | 🟡 MinorSkip trailing end timestamps for word-sync lines.
appendLrcLinestill appends[end]unconditionally whendurationMs > 0, so word-sync lines get both a trailing timestamp and a following<word:...>block. Keep the end timestamp for line-sync output only, and update the KDoc accordingly.Proposed direction
- /** Appends `[mm:ss.cc]<tag>text\n` */ - private fun StringBuilder.appendLrcLine(timeMs: Long, durationMs: Long, tag: String, text: String) { + /** Appends `[mm:ss.cc]<tag>text`, optionally followed by an end timestamp. */ + private fun StringBuilder.appendLrcLine( + timeMs: Long, + durationMs: Long, + tag: String, + text: String, + includeEndTime: Boolean = true, + ) { append(formatLrcTime(timeMs)) append(tag) append(text) - if (durationMs > 0) { + if (includeEndTime && durationMs > 0) { append(formatLrcTime(timeMs + durationMs)) }Then pass
includeEndTime = !isWordSyncfrom the word-sync-aware callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/lyrics/LyricsPlusProvider.kt` around lines 373 - 380, The appendLrcLine currently always appends an end timestamp when durationMs > 0 which causes word-sync lines to get a trailing end time plus a following <word:...> block; change appendLrcLine(timeMs: Long, durationMs: Long, tag: String, text: String) to accept a boolean flag (e.g. includeEndTime: Boolean) and only append the end timestamp when includeEndTime is true and durationMs > 0, update the KDoc to reflect that end timestamps are optional (e.g., "Appends `[mm:ss.cc]<tag>text` and optionally an end timestamp when includeEndTime is true"), and update the word-sync-aware callers to pass includeEndTime = !isWordSync so line-sync still includes end times and word-sync does not.
🧹 Nitpick comments (1)
app/src/main/kotlin/com/metrolist/music/lyrics/LyricsPlusProvider.kt (1)
307-315: Update the stale agent-mapping comment.The code now uses resolved aliases directly, but the comment still says aliases are remapped into free
v1/v2slots.Proposed cleanup
- // Agent mapping - // The JSON aliases (v1, v2, v1000) are used directly. Others get mapped - // to the next free v1/v2 slot, falling back to v1. + // Agent mapping: use resolved JSON aliases directly. val agentMap = linkedMapOf<String, String>() // raw alias -> lrc idAs per coding guidelines, comments should be added only for complex logic or non-obvious code and avoid redundant comments that simply restate what the code does.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/lyrics/LyricsPlusProvider.kt` around lines 307 - 315, The top-of-block comment describing agent mapping is stale: it claims aliases are remapped into v1/v2 slots, but the code in LyricsPlusProvider.kt now stores resolved aliases directly into agentMap (see agentMap, lyrics.forEach and the raw assignment). Update or remove the comment to reflect current behavior—either replace it with a concise note that agentMap maps raw/resolved singer aliases to LRC IDs (no remapping to v1/v2), or delete the redundant comment entirely so only non-obvious logic is described.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/lyrics/LyricsPlusProvider.kt`:
- Around line 273-278: The parse step can return only agent headers (no actual
lines) which allows TTMLParser.toLRC to emit nonblank agent-only output; after
calling TTMLParser.parseTTML and obtaining parsedLines (and agentsMap), check if
parsedLines is null or parsedLines.isEmpty() and if so return null so formatting
is skipped and fallback can run; specifically adjust the logic around
TTMLParser.parseTTML / parsedLines before invoking TTMLParser.toLRC to reject
agent-only parses.
- Around line 357-359: The code currently passes line.duration to appendLrcLine
which treats durationMs as relative to bgTime, causing overextended bg lines
when bgTime > line.time; instead compute the background duration from the
background word bounds by computing bgDuration = (bgToEmit.maxOf { it.time +
it.duration } - bgTime). Replace the use of line.duration in the appendLrcLine
call (the call in which bgTime, bgTag and bgText are used) with this bgDuration
so the passed duration is relative to bgTime and reflects the actual
background-word bounds.
---
Duplicate comments:
In `@app/src/main/kotlin/com/metrolist/music/lyrics/LyricsPlusProvider.kt`:
- Around line 373-380: The appendLrcLine currently always appends an end
timestamp when durationMs > 0 which causes word-sync lines to get a trailing end
time plus a following <word:...> block; change appendLrcLine(timeMs: Long,
durationMs: Long, tag: String, text: String) to accept a boolean flag (e.g.
includeEndTime: Boolean) and only append the end timestamp when includeEndTime
is true and durationMs > 0, update the KDoc to reflect that end timestamps are
optional (e.g., "Appends `[mm:ss.cc]<tag>text` and optionally an end timestamp
when includeEndTime is true"), and update the word-sync-aware callers to pass
includeEndTime = !isWordSync so line-sync still includes end times and word-sync
does not.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/lyrics/LyricsPlusProvider.kt`:
- Around line 307-315: The top-of-block comment describing agent mapping is
stale: it claims aliases are remapped into v1/v2 slots, but the code in
LyricsPlusProvider.kt now stores resolved aliases directly into agentMap (see
agentMap, lyrics.forEach and the raw assignment). Update or remove the comment
to reflect current behavior—either replace it with a concise note that agentMap
maps raw/resolved singer aliases to LRC IDs (no remapping to v1/v2), or delete
the redundant comment entirely so only non-obvious logic is described.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b6b768d-48ee-4e58-a64c-9e1fd6185f8e
📒 Files selected for processing (2)
app/src/main/kotlin/com/metrolist/music/lyrics/LyricsPlusProvider.ktapp/src/main/kotlin/com/metrolist/music/ui/component/LyricsLine.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/kotlin/com/metrolist/music/ui/component/LyricsLine.kt
appendLrcLine treats durationMs as relative to bgTime, so passing line.duration overextends background lines whenever bgTime > line.time.
With agentsMap passed into toLRC, an empty parsedLines list can still produce nonblank output because TTMLParser.toLRC emits [agent:...] headers before line content. Guard the parsed line list before formatting so fallback can still run.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/main/kotlin/com/metrolist/music/lyrics/LyricsPlusProvider.kt (1)
345-345:⚠️ Potential issue | 🟡 MinorSkip trailing end timestamps for word-sync lines.
appendLrcLinestill appends[end]wheneverdurationMs > 0, including lines followed by a word-sync block. This duplicates the existing concern: passisWordSyncinto the helper and omit the trailing timestamp for word-sync output.Proposed adjustment
- sb.appendLrcLine(line.time, line.duration, agentTag, mainText) + sb.appendLrcLine(line.time, line.duration, agentTag, mainText, isWordSync) @@ - sb.appendLrcLine(bgTime, bgDuration, bgTag, bgText) + sb.appendLrcLine(bgTime, bgDuration, bgTag, bgText, isWordSync) @@ - /** Appends `[mm:ss.cc]<tag>text\n` */ - private fun StringBuilder.appendLrcLine(timeMs: Long, durationMs: Long, tag: String, text: String) { + /** Appends `[mm:ss.cc]<tag>text[end]\n`; omits `[end]` for word-sync lines. */ + private fun StringBuilder.appendLrcLine( + timeMs: Long, + durationMs: Long, + tag: String, + text: String, + isWordSync: Boolean, + ) { append(formatLrcTime(timeMs)) append(tag) append(text) - if (durationMs > 0) { + if (!isWordSync && durationMs > 0) { append(formatLrcTime(timeMs + durationMs)) } append('\n')Also applies to: 364-364, 378-385
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/lyrics/LyricsPlusProvider.kt` at line 345, The appendLrcLine helper currently appends an end timestamp whenever durationMs > 0, causing duplicated trailing timestamps for lines that are followed by word-sync blocks; modify appendLrcLine to accept an additional boolean parameter (e.g., isWordSync) and, when isWordSync is true, skip adding the trailing end timestamp even if durationMs > 0. Update all callers (the invocations at/around the lines that call sb.appendLrcLine — including the calls at the sites referenced in the comment: the call at sb.appendLrcLine(line.time, line.duration, agentTag, mainText) and the other calls around the 364 and 378–385 regions) to pass the correct isWordSync flag based on whether a word-sync block follows, and adjust the helper signature and any related tests/uses accordingly.
🧹 Nitpick comments (1)
app/src/main/kotlin/com/metrolist/music/lyrics/LyricsPlusProvider.kt (1)
307-315: Update the stale agent-mapping comment.Line 314 now preserves the resolved singer alias directly, but the comment still says unknown aliases are remapped to free
v1/v2slots. Remove or rewrite it to avoid misleading future changes.Proposed cleanup
- // Agent mapping - // The JSON aliases (v1, v2, v1000) are used directly. Others get mapped - // to the next free v1/v2 slot, falling back to v1. + // Agent mapping: line.element.singer is already a resolved alias. val agentMap = linkedMapOf<String, String>() // raw alias -> lrc idAs per coding guidelines, "
**/*.kt: Follow best practices for Kotlin and Android development Comments should be added only for complex logic or non-obvious code. Avoid redundant comments that simply restate what the code does."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/lyrics/LyricsPlusProvider.kt` around lines 307 - 315, The comment above the agent mapping is stale and misleading because the code (the linkedMap agentMap and the lyrics.forEach block that sets agentMap[raw] = raw) now preserves the resolved singer alias directly instead of remapping unknown aliases into v1/v2 slots; update or remove that comment to accurately describe current behavior (e.g., "Map raw singer aliases to resolved lrc ids; unknown aliases are preserved as-is") and ensure any comment references the symbols agentMap and the lyrics.forEach/singer resolution logic so future readers understand that aliases are kept unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/main/kotlin/com/metrolist/music/lyrics/LyricsPlusProvider.kt`:
- Line 345: The appendLrcLine helper currently appends an end timestamp whenever
durationMs > 0, causing duplicated trailing timestamps for lines that are
followed by word-sync blocks; modify appendLrcLine to accept an additional
boolean parameter (e.g., isWordSync) and, when isWordSync is true, skip adding
the trailing end timestamp even if durationMs > 0. Update all callers (the
invocations at/around the lines that call sb.appendLrcLine — including the calls
at the sites referenced in the comment: the call at sb.appendLrcLine(line.time,
line.duration, agentTag, mainText) and the other calls around the 364 and
378–385 regions) to pass the correct isWordSync flag based on whether a
word-sync block follows, and adjust the helper signature and any related
tests/uses accordingly.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/lyrics/LyricsPlusProvider.kt`:
- Around line 307-315: The comment above the agent mapping is stale and
misleading because the code (the linkedMap agentMap and the lyrics.forEach block
that sets agentMap[raw] = raw) now preserves the resolved singer alias directly
instead of remapping unknown aliases into v1/v2 slots; update or remove that
comment to accurately describe current behavior (e.g., "Map raw singer aliases
to resolved lrc ids; unknown aliases are preserved as-is") and ensure any
comment references the symbols agentMap and the lyrics.forEach/singer resolution
logic so future readers understand that aliases are kept unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 294c23d9-68e5-4122-8849-712317fea9bc
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/lyrics/LyricsPlusProvider.kt
…gnificantly *When the phone sleeps and wakes up, the position tracking loop, (LaunchedEffect(lyrics, lines)) resumes, but there's a time-jump issue: lastUpdateTime still holds the old timestamp from before it randomly got stopped. This means elapsed = now - lastUpdateTime becomes huge (potentially minutes), causing the interpolated position to spike way ahead.
ibratabian17
commented
Apr 25, 2026
Contributor
Author
ibratabian17
left a comment
ibratabian17
left a comment
There was a problem hiding this comment.
all major issue fixed
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Not romanized correctly
Weird text spacing
If a song has only a few romanizable words, it will all be romanized
Lyrics aren't scrollable if they aren't synced, lyrics are overall hard to scroll If you could also fix the lyrics not fetching properly that would be nice. Prioritize betterlyrics and lrclib |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Cause
Solution
Parser Expansion: Added Line-sync riched timing support.
Testing
Tested on POCO F1
Related Issues
Summary by CodeRabbit
New Features
Improvements